-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix shell feature discovery and remove duplicate service registrations #7285
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…es, and improve security Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>
Greptile OverviewGreptile SummaryThis PR addresses critical runtime DI failures and improves the shell feature system integrity by adding missing Key Changes:
The changes are well-structured and follow proper separation of concerns by moving duplicate registrations to dedicated feature modules. Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| src/common/Elsa.Features/Implementations/Module.cs | Added IInstalledFeatureProvider registration for API endpoint compatibility with legacy module system |
| src/modules/Elsa.Identity/ShellFeatures/IdentityFeature.cs | Removed hard-coded signing key default value; applications must now configure SigningKey explicitly to prevent security issues |
| src/modules/Elsa.WorkflowProviders.BlobStorage/ShellFeatures/BlobStorageFeature.cs | Fixed null-safety: now uses AppContext.BaseDirectory fallback when Assembly.GetEntryAssembly() returns null |
| src/modules/Elsa.Workflows.Management/ShellFeatures/WorkflowManagementFeature.cs | Removed caching service registrations and decorators (moved to CachingWorkflowDefinitionsFeature) |
| src/modules/Elsa.Workflows.Runtime/ShellFeatures/WorkflowRuntimeFeature.cs | Removed conflicting recurring task schedule, duplicate WorkflowResumer and BookmarkQueueWorker registrations, and caching decorators (moved to CachingWorkflowRuntimeFeature) |
Sequence Diagram
sequenceDiagram
participant App as Application Startup
participant Module as Module System
participant WM as WorkflowManagementFeature
participant WR as WorkflowRuntimeFeature
participant CWD as CachingWorkflowDefinitionsFeature
participant CWR as CachingWorkflowRuntimeFeature
participant Shell as Shell Features (7 new attributes)
Note over App,Shell: Before PR: Duplicate Registrations
App->>Module: Configure Features
Module->>WM: ConfigureServices()
WM->>WM: Register IWorkflowDefinitionStore
WM->>WM: Register Caching Decorators ❌
Module->>WR: ConfigureServices()
WR->>WR: Register WorkflowResumer (twice) ❌
WR->>WR: Register BookmarkQueueWorker (twice) ❌
WR->>WR: Configure TriggerBookmarkQueue (10s) ❌
WR->>WR: Register TriggerBookmarkQueue (1m) ❌
WR->>WR: Register Caching Decorators ❌
Note over App,Shell: After PR: Clean Separation
App->>Module: Configure Features
Module->>Shell: Resolve DependsOn references ✓
Shell->>Shell: ShellFeature attributes enable resolution
Module->>WM: ConfigureServices()
WM->>WM: Register IWorkflowDefinitionStore only ✓
Module->>CWD: ConfigureServices()
CWD->>CWD: Register Caching Decorators ✓
Module->>WR: ConfigureServices()
WR->>WR: Register WorkflowResumer (once) ✓
WR->>WR: Register BookmarkQueueWorker (once) ✓
WR->>WR: Register TriggerBookmarkQueue (1m) ✓
Module->>CWR: ConfigureServices()
CWR->>CWR: Register Caching Decorators ✓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
17 files reviewed, 1 comment
| // Configure options - Note: SigningKey must be configured by the application for security | ||
| services.Configure<IdentityTokenOptions>(_ => { }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking change - existing applications using IdentityFeature will need to explicitly configure the SigningKey in their startup code, otherwise token generation/validation will fail at runtime. Consider documenting migration steps.
* Add CShells package references and integrate shell features into the application * Annotate shell features with `[ShellFeature]` attribute and update `TempElsaFeature` to use `ConfigureElsa`. * Revert "Annotate shell features with `[ShellFeature]` attribute and update `TempElsaFeature` to use `ConfigureElsa`." This reverts commit e8a875e. * Introduce `Elsa.ModularServer.Web` with a minimal API, restructure shell feature configuration, and remove obsolete `CShells` dependency * Update `CShells` package references, add Fody weaver, and configure new CShells package sources in `NuGet.Config`. * Adds CShells integration to Elsa Integrates CShells to enhance modularity and extensibility. - Adds CShells related projects to the solution. - Updates NuGet configuration to include CShells preview feed. - Creates initial app settings for CShells configuration. - Adds CShells.AspNetCore project reference. - Implements CShells extensions in the program file. - Creates shell feature classes in Elsa.Common. - Creates shell feature classes in Elsa.Expressions. - Creates shell feature classes in Elsa.Workflows.Core. - Creates shell feature classes in Elsa.Workflows.Management. - Creates shell feature classes in Elsa.Workflows.Runtime. - Creates shell feature classes in Elsa module. * Refactor CShells: enhance pipeline configuration and features Consolidated updates to CShells including a new `ResolverPipelineBuilder` for customizable resolver strategy pipelines. Improved assembly scanning, error handling in web routing, and streamlined shell feature dependencies for better clarity and functionality. * Add feature registration system and FastEndpoints integration Introduced a feature registration infrastructure with `IInstalledFeatureProvider` and related implementations. Added shell-based feature configurations such as caching, SAS tokens, workflows management, and a FastEndpoints integration module to support dynamic API registration. * Refactor shell routing and enhance global route handling Refactored `ShellEndpointRouteBuilder` to simplify initialization and support a combined shell/global route prefix. Enhanced `ShellEndpointRegistrationHandler` to include global route prefix logic and improved feature discovery using pre-resolved descriptors. Updated `Program.cs` for consistent middleware setup. * Refactor feature endpoints to use `IInstalledFeatureProvider` for improved dependency management and simplified implementation * Add display names, descriptions, and dependency enhancements to shell features Standardized `ShellFeature` attributes across `ElsaFeature`, `WorkflowRuntimeFeature`, and `WorkflowManagementFeature` by adding display names, descriptions, and improving dependency declarations. Updated `ElsaFeature` to register `IInstalledFeatureProvider` for feature bridging. * Add FastEndpoints references and update package versions Added project references to CShells.FastEndpoints and related projects in multiple csproj files. Updated FastEndpoints package versions in `Directory.Packages.props` for compatibility with .NET 8/9/10. Removed obsolete folder references from Elsa.Caching.csproj and refined the namespace in CShells.AspNetCore.Abstractions. * Add Identity and DefaultAuthentication features to appsettings.json configuration * Add project references for Elsa.Identity and CShells.FastEndpoints.Abstractions * Add `Identity` and `DefaultAuthentication` shell features with enhanced authentication and authorization support * Set default signing key in `IdentityTokenOptions` for identity configuration * Add service exclusion infrastructure for shell-specific contexts Introduce `IShellServiceExclusionProvider` and `IShellServiceExclusionRegistry` to manage excluded service types per-shell. Implement ASP.NET Core-specific providers for authentication and authorization to enable shell-specific configurations. Refactor `DefaultShellHost` to use the new exclusion registry for service inheritance filtering. * Refactor CShells authentication and authorization APIs Renamed and unified methods for shell authentication and authorization and added a new combined method `WithAuthenticationAndAuthorization`. Enhanced `AddShells` to automatically register a default configuration provider if none is specified. Updated usage in Elsa.ModularServer to utilize the new API. * Add Elsa-specific FastEndpoints configurator and feature Introduce `ElsaFastEndpointsConfigurator` to customize FastEndpoints serialization and value parsing for Elsa workflows. Register this functionality through the new `ElsaFastEndpointsFeature`, which integrates with the shell's dependency injection system using an `IFastEndpointsConfigurator` interface. * Update Workflow API feature dependency to `ElsaFastEndpoints` * Pass `cancellationToken` to `ReadToEndAsync` in `PostEndpoint` for improved request handling. * Add project references for CShells.AspNetCore and CShells.FastEndpoints.Abstractions * Update CShells package versions to `0.0.6-preview.30` and add `CShells.FastEndpoints.Abstractions` * Configures shell routing and features Enables path routing for shells to allow proper routing within each shell. Configures the ElsaFastEndpoints feature to depend on the FastEndpoints feature. This ensures that FastEndpoints is properly configured before Elsa's FastEndpoints configurations are applied. Registers activity types within the WorkflowManagementFeature. This ensures activities are available for workflow construction and execution. * Add shell lifecycle management and notification handlers Introduced interfaces and handlers for shell activation (`IShellActivatedHandler`) and deactivation (`IShellDeactivatingHandler`) to manage shell lifecycles. Added `ShellStartupHostedService` to coordinate shell activation on startup and deactivation on shutdown. Updated notification system to support new shell lifecycle events and renamed existing notification records for consistency. * Introduces EF Core persistence layer Adds base classes and implementations for EF Core persistence, including database provider configuration and shell feature integration. This change introduces a generic approach to configuring EF Core persistence for various Elsa modules, promoting code reuse and simplifying the process of supporting different database providers. It includes: - Base classes for database provider configurators and shell features. - Implementations for Sqlite, SQL Server, MySql, PostgreSql, and Oracle. - Shell features for Alterations, Identity, Labels, Management (Workflow Definitions and Instances), Runtime, and Tenants modules. * Add comprehensive feature configuration validation system Introduce a feature configuration system with support for binding, auto-configuration, and validation using DataAnnotations, FluentValidation, and composite patterns. Includes new validators, binding logic, and extensions to simplify configuration tasks while ensuring robustness and flexibility. * Add persistence shell features for MySql, Oracle, PostgreSql, and Sqlite Introduced new shell features to configure MySql, Oracle, PostgreSql, and Sqlite persistence for workflow definitions and runtime data. Updated `appsettings.json` to replace individual Sqlite features with a unified `SqliteWorkflowPersistence`. Made minor code cleanup in `FastEndpointsFeature`. * Configure shell features for persistence Added `IServiceCollection` configuration for MySql, Oracle, PostgreSql, and Sqlite shell features to set up persistence services. * Remove DatabaseProviderConfigurators and refactor persistence shell features Deleted DatabaseProviderConfigurator classes and restructured persistence shell features by integrating direct configuration logic for MySql, Oracle, PostgreSql, Sqlite, and SqlServer. Simplified configuration by inheriting from abstract shell feature base classes and removed redundant code. * Correct IWorkflowDefinitionPublisher registration to use WorkflowDefinitionPublisher implementation * Add resilience feature and scoped services configuration for Sqlite persistence - Integrated `Microsoft.Extensions.DependencyInjection` to shell features for Sqlite persistence. - Updated `appsettings.json` and project references to include a new 'Resilience' feature. - Changed `ICommitStateHandler` service registration in `WorkflowRuntimeFeature` to use an implementation. * Add `ResilienceShellFeature` for configuring resilience strategies - Implemented new `ResilienceShellFeature` class to manage services related to resilience features. - Added scoped and singleton service registrations for resilience strategies, exception detection, and activity invocation. - Configured expression options for resilience handling in workflows. * Add new shell features: Alterations, Blob Storage, Caching, Clustering, CSharp, Distributed Runtime, ElsaScript, Flowchart, HTTP, JavaScript, Key-Value, and Labels * Switch project references to package references for CShells libraries and update to version 0.0.7. * Potential fix for pull request finding 'Call to 'System.IO.Path.Combine' may silently drop its earlier arguments' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> * Fix shell feature discovery and remove duplicate service registrations (#7285) * Initial plan * Address PR review comments: Add ShellFeature attributes, fix duplicates, and improve security Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> * Update dependencies and fix scoped services registration in `WorkflowRuntimeFeature` * Fix null reference and typo in shell feature provider (#7286) * Initial plan * Fix null StartupType guard in Find() and typo in variable descriptor Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> --------- Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>
Addresses review feedback from #7279 to fix runtime DI failures and improve shell feature system integrity.
Feature Discovery
Added missing
[ShellFeature]attributes to 7 features (Mediator, Multitenancy, SystemClock, DefaultFormatters, StringCompression, CommitStrategies, Expressions). Without these attributes, CShells cannot resolveDependsOnreferences, causing runtime failures.Duplicate Registration Fixes
WorkflowManagementFeatureandWorkflowRuntimeFeature— moved exclusively toCachingWorkflowDefinitionsFeatureandCachingWorkflowRuntimeFeatureto prevent double-decorationTriggerBookmarkQueueRecurringTaskschedule (10s vs 1m)WorkflowResumerandBookmarkQueueWorkerregistrationsITypeDefinitionServiceregistrationWorkflowsFeature(already inFlowchartFeature)Security & Compatibility
IdentityFeature— must be configured by application to prevent token forgeryBlobStorageFeature.GetDefaultWorkflowsDirectoryusingAppContext.BaseDirectoryfallbackIInstalledFeatureProviderin legacy module system for API endpoint compatibilityPersistenceShellFeatureBase(Scoped vs Singleton)Cleanup
Removed commented-out
CShells.Abstractionspackage references from Elsa.Common and Elsa.Workflows.Core projects.💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.